Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use lobstr implementation of object_size #488

Merged
merged 4 commits into from
Aug 29, 2024
Merged

Use lobstr implementation of object_size #488

merged 4 commits into from
Aug 29, 2024

Conversation

dfalbel
Copy link
Contributor

@dfalbel dfalbel commented Aug 28, 2024

Implements object_size as a port of the lobstr implementation, which should be more resilient to ALTREP objects, although it also:

  • Accounts for all types of shared values, not just strings in the global string pool.
  • Includes the size of environments (up to env)

It fixes issues that can be observed in: posit-dev/positron#4512

@juliasilge
Copy link
Contributor

Does this also address posit-dev/positron#478?

@dfalbel
Copy link
Contributor Author

dfalbel commented Aug 28, 2024

Yes!

@dfalbel dfalbel requested a review from lionel- August 28, 2024 19:56
Copy link
Contributor

@lionel- lionel- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

I see you also ported the lobstr tests, which is nice.

Ideally we wouldn't have two implementations, but it seems hard to share it since a package can't depend on harp. Maybe we should split harp into CRAN-able and un-CRAN-able parts. That said there's also no guarantee that lobstr can remain on CRAN in the long term.

We could also have done it the other way around, e.g. using https://github.com/rust-lang/cc-rs to build the lobstr sources as part of our build.

But let's worry about all that if the maintenance of this Rust port turns out to be a burden.

crates/harp/src/utils.rs Outdated Show resolved Hide resolved
crates/harp/src/size.rs Outdated Show resolved Hide resolved
crates/harp/src/size.rs Outdated Show resolved Hide resolved
crates/harp/src/size.rs Outdated Show resolved Hide resolved
@dfalbel
Copy link
Contributor Author

dfalbel commented Aug 29, 2024

It would be nice to use a single source of truth but seemed more work to setup the build system than just porting to rust. The object_size code in lobstr hasn't been changed in 2 years, so I guess it won't be too complicated to maintain the port:

image

@dfalbel dfalbel merged commit 4ba354c into main Aug 29, 2024
3 checks passed
@dfalbel dfalbel deleted the bugfix/r-size branch August 29, 2024 12:04
@github-actions github-actions bot locked and limited conversation to collaborators Aug 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants